Fix handling of empty payment IDs#621
Conversation
📝 WalkthroughWalkthroughRemoved an early-return in WidgetContainer's visibility-change handler so visibility-triggered transaction checks run even when Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react/lib/tests/util/validate.test.ts (1)
42-85:⚠️ Potential issue | 🔴 CriticalDuplicate
constdeclarations will fail TypeScript compilationInside the single
it()callback,expectedPaymentIdis declaredconsttwice (lines 52 and 71) andresultis declaredconsttwice (lines 58 and 73). TypeScript reportsTS2451: Cannot redeclare block-scoped variablefor both, soyarn testwill fail to compile this test.Split the two scenarios into separate
it()blocks, or wrap the second scenario in its own block scope:🐛 Proposed fix – split into two `it` blocks
- it('true if disablePaymentId is set even if both paymentId and received paymentId are empty or undefined', async () => { + it('true if disablePaymentId is set when received paymentId is empty and expectedPaymentId is undefined', async () => { const transaction: Transaction = { amount: '101.00', paymentId: '', message: 'test opReturn message', rawMessage: 'test opReturn message', hash: '', timestamp: 0, address: 'ecash:qrmm7edwuj4jf7tnvygjyztyy0a0qxvl7quss2vxek', }; const expectedPaymentId = undefined; const expectedAmount = resolveNumber('101.00'); const expectedOpReturn = 'test opReturn message'; const price = 1; const currency = 'XEC'; const result = shouldTriggerOnSuccess( transaction, currency, price, /*randomSatoshis=*/ false, /*disablePaymentId=*/ true, expectedPaymentId, expectedAmount, expectedOpReturn ); expect(result).toBe(true); - - const expectedPaymentId = ''; - - const result = shouldTriggerOnSuccess( - transaction, - currency, - price, - /*randomSatoshis=*/ false, - /*disablePaymentId=*/ true, - expectedPaymentId, - expectedAmount, - expectedOpReturn - ); - - expect(result).toBe(true); }); + + it('true if disablePaymentId is set when both expected and received paymentId are empty strings', async () => { + const transaction: Transaction = { + amount: '101.00', + paymentId: '', + message: 'test opReturn message', + rawMessage: 'test opReturn message', + hash: '', + timestamp: 0, + address: 'ecash:qrmm7edwuj4jf7tnvygjyztyy0a0qxvl7quss2vxek', + }; + const expectedPaymentId = ''; + const expectedAmount = resolveNumber('101.00'); + const expectedOpReturn = 'test opReturn message'; + const price = 1; + const currency = 'XEC'; + + const result = shouldTriggerOnSuccess( + transaction, + currency, + price, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ true, + expectedPaymentId, + expectedAmount, + expectedOpReturn + ); + + expect(result).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/tests/util/validate.test.ts` around lines 42 - 85, The test currently redeclares block-scoped constants (expectedPaymentId and result) twice inside a single it() causing TS2451; split the two scenarios into separate it() blocks (e.g., two it(...) with descriptive names) or wrap the second scenario in its own block scope and rename/redeclare variables (or change const to let) so each shouldTriggerOnSuccess(...) call uses uniquely declared expectedPaymentId and result variables; update the test surrounding the Transaction object and calls to shouldTriggerOnSuccess to ensure no duplicate const declarations remain.
🧹 Nitpick comments (2)
react/lib/tests/util/validate.test.ts (2)
375-417: FIXME documents a known behavioral gap:randomSatoshis=trueunconditionally bypassespaymentIdvalidationThe comment at lines 375-382 accurately diagnoses the issue: when
randomSatoshisis truthy, the innerif(!randomSatoshis || randomSatoshis === 0)block (validate.ts line 49) is skipped entirely, soisPaymentIdValidremainstrueregardless of whatexpectedPaymentIdthe caller supplies. The test at line 383 locks in this (undesirable) behavior. Consider tracking this as a separate issue so it doesn't get lost.Would you like me to open a new issue to track removing
randomSatoshisor fixing the payment ID bypass?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/tests/util/validate.test.ts` around lines 375 - 417, The test shows randomSatoshis=true currently bypasses paymentId validation; update shouldTriggerOnSuccess (in validate.ts) so the paymentId check (isPaymentIdValid) is performed regardless of randomSatoshis: keep the randomSatoshis condition only for amount deviation logic, but always evaluate paymentId when disablePaymentId is false and expectedPaymentId is provided (i.e., move or duplicate the paymentId validation out of the if(!randomSatoshis || randomSatoshis === 0) block and ensure isPaymentIdValid is set based on transaction.paymentId === expectedPaymentId).
346-350: Pre-existingexpectedAmountdesign quirk is now documented but left unfixedThe FIXME correctly identifies that in the cross-currency branch of
validate.ts(lines 35-38),expectedAmountis unused — the comparison is betweencurrencyObject.float / priceandtransaction.amount. TheexpectedAmountparameter only serves as a truthiness gate for the wholeif(expectedAmount)block (line 29). This means:
- Passing
expectedAmount = resolveNumber('101.00')works only because it's truthy, not because its value matters.- Any caller that omits
expectedAmountwould silently skip amount validation entirely for the USD-currency path.Consider opening a separate issue to decouple the "run amount validation" gate from the
expectedAmountvalue itself (e.g., a dedicatedvalidateAmountflag or usingcurrencyObjectpresence as the trigger).Would you like me to open a tracking issue for this design quirk?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/tests/util/validate.test.ts` around lines 346 - 350, The test reveals expectedAmount is only used as a truthiness gate, so update the implementation in validate.ts (the cross-currency branch used by shouldTriggerOnSuccess) to decouple "run amount validation" from the expectedAmount value: introduce a dedicated boolean flag (e.g., validateAmount or use currencyObject presence) that controls whether amount checks run, and change the condition (currently if (expectedAmount) ...) to check that flag instead; ensure shouldTriggerOnSuccess and tests in react/lib/tests/util/validate.test.ts pass a proper boolean or rely on currencyObject so callers can omit expectedAmount without skipping validation, and update any call sites that currently rely on truthiness of resolveNumber('101.00') accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@react/lib/tests/util/validate.test.ts`:
- Around line 42-85: The test currently redeclares block-scoped constants
(expectedPaymentId and result) twice inside a single it() causing TS2451; split
the two scenarios into separate it() blocks (e.g., two it(...) with descriptive
names) or wrap the second scenario in its own block scope and rename/redeclare
variables (or change const to let) so each shouldTriggerOnSuccess(...) call uses
uniquely declared expectedPaymentId and result variables; update the test
surrounding the Transaction object and calls to shouldTriggerOnSuccess to ensure
no duplicate const declarations remain.
---
Nitpick comments:
In `@react/lib/tests/util/validate.test.ts`:
- Around line 375-417: The test shows randomSatoshis=true currently bypasses
paymentId validation; update shouldTriggerOnSuccess (in validate.ts) so the
paymentId check (isPaymentIdValid) is performed regardless of randomSatoshis:
keep the randomSatoshis condition only for amount deviation logic, but always
evaluate paymentId when disablePaymentId is false and expectedPaymentId is
provided (i.e., move or duplicate the paymentId validation out of the
if(!randomSatoshis || randomSatoshis === 0) block and ensure isPaymentIdValid is
set based on transaction.paymentId === expectedPaymentId).
- Around line 346-350: The test reveals expectedAmount is only used as a
truthiness gate, so update the implementation in validate.ts (the cross-currency
branch used by shouldTriggerOnSuccess) to decouple "run amount validation" from
the expectedAmount value: introduce a dedicated boolean flag (e.g.,
validateAmount or use currencyObject presence) that controls whether amount
checks run, and change the condition (currently if (expectedAmount) ...) to
check that flag instead; ensure shouldTriggerOnSuccess and tests in
react/lib/tests/util/validate.test.ts pass a proper boolean or rely on
currencyObject so callers can omit expectedAmount without skipping validation,
and update any call sites that currently rely on truthiness of
resolveNumber('101.00') accordingly.
When the expected payment ID is empty, it's possible to match against existing transaction even if they are not paybutton transactions at all. If the payment ID is not set it should not be relied upon. Update and clarify the tests accordingly. This update allows for code simplification by removing a workaround from the widget container. Test Plan: yarn test
Fabcien
force-pushed
the
dont_match_undefined_paymentid
branch
from
b2d2939 to
9f9d52b
Compare
|
Updated to fix some variable shadowing in the test |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react/lib/util/validate.ts`:
- Around line 50-51: WidgetContainer must honor the implicit contract that
payment checks are disabled when no paymentId will ever be provided; update the
WidgetContainer props normalization so that if props.paymentId is
missing/undefined (and WidgetContainer will not call getPaymentId itself) you
force disablePaymentId to true (i.e., set the value used by validate.ts’s
isPaymentIdValid calculation). Concretely, in the WidgetContainer initialization
(where isPropsTrue(props.disablePaymentId) is used) detect absence of
props.paymentId (and/or lack of a call to getPaymentId) and override/assign
disablePaymentId = true so isPaymentIdValid (and the logic around
expectedPaymentId/paymentId matching) does not block transactions when no
paymentId exists.
| const paymentIdsMatch = !!expectedPaymentId && expectedPaymentId === paymentId; | ||
| isPaymentIdValid = disablePaymentId ? true : paymentIdsMatch; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts -A 10 -B 2 'shouldTriggerOnSuccess\s*\('Repository: PayButton/paybutton
Length of output: 15179
🏁 Script executed:
rg -n --type=ts -C 5 'disablePaymentId'Repository: PayButton/paybutton
Length of output: 50375
🏁 Script executed:
sed -n '87,113p' react/lib/tests/util/validate.test.tsRepository: PayButton/paybutton
Length of output: 975
🏁 Script executed:
sed -n '42,85p' react/lib/tests/util/validate.test.tsRepository: PayButton/paybutton
Length of output: 1407
🏁 Script executed:
sed -n '87,115p' react/lib/tests/util/validate.test.tsRepository: PayButton/paybutton
Length of output: 1131
🏁 Script executed:
# Also check the full validate.ts function to understand all validation steps
cat -n react/lib/util/validate.ts | head -65Repository: PayButton/paybutton
Length of output: 2735
🏁 Script executed:
# Check how PayButton sets up the checkSuccessInfo and what paymentId/disablePaymentId values flow through
sed -n '130,320p' react/lib/components/PayButton/PayButton.tsx | grep -A 50 -B 5 'checkSuccessInfo'Repository: PayButton/paybutton
Length of output: 645
🏁 Script executed:
# Check how thisPaymentId is determined in PayButton
sed -n '130,250p' react/lib/components/PayButton/PayButton.tsx | grep -B 10 -A 3 'thisPaymentId\|paymentId ='Repository: PayButton/paybutton
Length of output: 45
🏁 Script executed:
# Look at PayButton's useState and variable initialization for paymentId and disablePaymentId
sed -n '64,130p' react/lib/components/PayButton/PayButton.tsxRepository: PayButton/paybutton
Length of output: 2377
🏁 Script executed:
# Check WidgetContainer logic around line 111 and how it determines disablePaymentId
sed -n '106,220p' react/lib/components/Widget/WidgetContainer.tsxRepository: PayButton/paybutton
Length of output: 3742
🏁 Script executed:
# Check the test expectations more carefully - does it expect true or false?
sed -n '87,130p' react/lib/tests/util/validate.test.ts | tail -40Repository: PayButton/paybutton
Length of output: 1488
🏁 Script executed:
# Check PayButton getPaymentId logic to confirm when paymentId is actually set
sed -n '136,162p' react/lib/components/PayButton/PayButton.tsxRepository: PayButton/paybutton
Length of output: 872
🏁 Script executed:
# Check WidgetContainer initialization of paymentId and disablePaymentId flow
sed -n '154,250p' react/lib/components/Widget/WidgetContainer.tsx | grep -A 3 -B 3 'paymentId\|disablePaymentId'Repository: PayButton/paybutton
Length of output: 505
🏁 Script executed:
# Check what isPropsTrue does
rg -n 'function isPropsTrue|const isPropsTrue' react/lib/Repository: PayButton/paybutton
Length of output: 149
🏁 Script executed:
# Check isPropsTrue implementation
sed -n '66,75p' react/lib/util/format.tsRepository: PayButton/paybutton
Length of output: 282
🏁 Script executed:
# Check if Widget/WidgetContainer has logic to fetch paymentId like PayButton does
rg -n 'getPaymentId\|createPayment' react/lib/components/Widget/Repository: PayButton/paybutton
Length of output: 45
🏁 Script executed:
# Check if there's a way paymentId gets generated in WidgetContainer - check for useEffect with paymentId
sed -n '300,450p' react/lib/components/Widget/WidgetContainer.tsx | grep -A 20 'useEffect'Repository: PayButton/paybutton
Length of output: 1509
🏁 Script executed:
# Check Widget.tsx to see if it has paymentId generation logic
rg -n 'getPaymentId\|createPayment' react/lib/components/Widget/Widget.tsxRepository: PayButton/paybutton
Length of output: 45
🏁 Script executed:
# Search for where internalPaymentId gets set in WidgetContainer or if there's initialization logic
rg -n 'setInternalPaymentId\|setPaymentId' react/lib/components/Widget/WidgetContainer.tsxRepository: PayButton/paybutton
Length of output: 45
🏁 Script executed:
# Check if WidgetContainer has any effect that initializes paymentId
sed -n '250,400p' react/lib/components/Widget/WidgetContainer.tsx | head -80Repository: PayButton/paybutton
Length of output: 2127
🏁 Script executed:
# Get the full context of WidgetContainer to understand the complete flow from start to when shouldTriggerOnSuccess is called
wc -l react/lib/components/Widget/WidgetContainer.tsxRepository: PayButton/paybutton
Length of output: 114
🏁 Script executed:
# Check the signature and props of WidgetContainer to see if paymentId is passed in
sed -n '20,50p' react/lib/components/Widget/WidgetContainer.tsxRepository: PayButton/paybutton
Length of output: 981
🏁 Script executed:
# Check how PayButton initializes and manages the socket setup with checkSuccessInfo
sed -n '290,320p' react/lib/components/PayButton/PayButton.tsxRepository: PayButton/paybutton
Length of output: 868
🏁 Script executed:
# Final check: look at the test setup for the critical case - what does the test comment say?
sed -n '87,120p' react/lib/tests/util/validate.test.tsRepository: PayButton/paybutton
Length of output: 1340
🏁 Script executed:
# Verify the complete picture - check if there are any other callers besides PayButton and WidgetContainer
rg -n 'new WidgetContainer\|<WidgetContainer' react/lib/Repository: PayButton/paybutton
Length of output: 45
Verify that WidgetContainer honors the implicit disablePaymentId = true contract when expectedPaymentId is absent.
The fix correctly prevents false positives by requiring expectedPaymentId to be truthy before matching. However, it introduces a critical implicit contract: when expectedPaymentId is absent or empty and disablePaymentId is false (its default), isPaymentIdValid becomes false, permanently blocking all transactions from triggering success.
This is safe in PayButton (which calls getPaymentId() to either fetch a valid paymentId or return undefined when disablePaymentId is true), but WidgetContainer violates it. WidgetContainer defaults disablePaymentId to false via isPropsTrue(props.disablePaymentId) (line 111) and accepts paymentId as an optional prop without fetching it. When WidgetContainer is instantiated without providing a paymentId and without explicitly setting disablePaymentId = true, every transaction will be rejected.
The test at lines 87-120 confirms this contract is intentional—the comment states: "Payment IDs are not set when the button is built so any tx with the same address and amount could match." Ensure WidgetContainer always sets disablePaymentId = true when no paymentId is provided or will be fetched.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react/lib/util/validate.ts` around lines 50 - 51, WidgetContainer must honor
the implicit contract that payment checks are disabled when no paymentId will
ever be provided; update the WidgetContainer props normalization so that if
props.paymentId is missing/undefined (and WidgetContainer will not call
getPaymentId itself) you force disablePaymentId to true (i.e., set the value
used by validate.ts’s isPaymentIdValid calculation). Concretely, in the
WidgetContainer initialization (where isPropsTrue(props.disablePaymentId) is
used) detect absence of props.paymentId (and/or lack of a call to getPaymentId)
and override/assign disablePaymentId = true so isPaymentIdValid (and the logic
around expectedPaymentId/paymentId matching) does not block transactions when no
paymentId exists.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
react/lib/tests/util/validate.test.ts (1)
375-382: FIXME documents a real behavioral defect —randomSatoshis=truesilently skipspaymentIdvalidation.In
validate.ts, thepaymentIdsMatchcheck is guarded byif(!randomSatoshis || randomSatoshis === 0), so settingrandomSatoshis=truebypassespaymentIdverification entirely even whendisablePaymentId=falseand a concreteexpectedPaymentIdis provided. The test correctly pins the current (broken) behavior, but this is a latent correctness issue: a payment with the wrongpaymentIdcan be accepted as long asrandomSatoshisis enabled.The FIXME comment is good for traceability, but consider opening a dedicated issue so this isn't lost.
Would you like me to draft a fix for
shouldTriggerOnSuccessthat applies thepaymentIdcheck independently ofrandomSatoshis, or open a tracking issue?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/tests/util/validate.test.ts` around lines 375 - 382, The test reveals that shouldTriggerOnSuccess in validate.ts currently skips paymentId validation when randomSatoshis is truthy because paymentIdsMatch is only checked under if(!randomSatoshis || randomSatoshis === 0); change the logic so paymentId validation (using paymentIdsMatch, expectedPaymentId and disablePaymentId) runs independently of randomSatoshis: remove or relocate the randomSatoshis guard so that when disablePaymentId is false and an expectedPaymentId is provided the paymentIdsMatch check is always performed, and only the amount/price entropy logic remains conditional on randomSatoshis.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react/lib/tests/util/validate.test.ts`:
- Around line 451-452: The test name uses printf `%s` placeholders which render
object rows as “[object Object]”; update the `test.each(cases)` title to use
Jest’s object interpolation with the actual row property names — replace
'expectedPaymentId=%s tx.paymentId=%s -> %s' with something like
'expectedPaymentId=$expectedPaymentId tx.paymentId=$tx.paymentId -> $expected'
(using the real property name used in `cases` for the final field) so each
object-based row produces unique, readable test names.
---
Nitpick comments:
In `@react/lib/tests/util/validate.test.ts`:
- Around line 375-382: The test reveals that shouldTriggerOnSuccess in
validate.ts currently skips paymentId validation when randomSatoshis is truthy
because paymentIdsMatch is only checked under if(!randomSatoshis ||
randomSatoshis === 0); change the logic so paymentId validation (using
paymentIdsMatch, expectedPaymentId and disablePaymentId) runs independently of
randomSatoshis: remove or relocate the randomSatoshis guard so that when
disablePaymentId is false and an expectedPaymentId is provided the
paymentIdsMatch check is always performed, and only the amount/price entropy
logic remains conditional on randomSatoshis.
| test.each(cases)( | ||
| 'expectedPaymentId=%s tx.paymentId=%s -> %s', |
There was a problem hiding this comment.
%s placeholders produce identical, unreadable test names for object-based test.each rows.
Jest's $variable syntax is required for object rows, and "you cannot use $variable with the printf formatting except for %%" — i.e. %s is only for Array<Array> rows where each element maps to a positional argument. With Array<Object> rows, all three %s resolve to [object Object], so all four matrix cases will surface with the same name when one fails, making it impossible to identify which combination broke.
🐛 Proposed fix — use `$propertyName` syntax
- 'expectedPaymentId=%s tx.paymentId=%s -> %s',
+ 'expectedPaymentId=$expLabel tx.paymentId=$txLabel -> $expected',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.each(cases)( | |
| 'expectedPaymentId=%s tx.paymentId=%s -> %s', | |
| test.each(cases)( | |
| 'expectedPaymentId=$expLabel tx.paymentId=$txLabel -> $expected', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react/lib/tests/util/validate.test.ts` around lines 451 - 452, The test name
uses printf `%s` placeholders which render object rows as “[object Object]”;
update the `test.each(cases)` title to use Jest’s object interpolation with the
actual row property names — replace 'expectedPaymentId=%s tx.paymentId=%s -> %s'
with something like 'expectedPaymentId=$expectedPaymentId
tx.paymentId=$tx.paymentId -> $expected' (using the real property name used in
`cases` for the final field) so each object-based row produces unique, readable
test names.
3 participants
When the expected payment ID is empty, it's possible to match against existing transaction even if they are not paybutton transactions at all. If the payment ID is not set it should not be relied upon.
Update and clarify the tests accordingly.
This update allows for code simplification by removing a workaround from the widget container.
Test Plan:
yarn test
Summary by CodeRabbit
Bug Fixes
Tests